Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skip dyn keyword lint under macros #59463

Merged

Conversation

pnkfelix
Copy link
Member

This PR is following my own intuition that rustfix should never inject bugs into working code (even if that comes at the expense of it failing to fix things that will become bugs).

Fix #56327

The existing `KeywordIdents` lint blindly scans the token stream for a
macro or macro definition. It does not attempt to parse the input,
which means it cannot distinguish between occurrences of `dyn` that
are truly instances of it as an identifier (e.g. `let dyn = 3;`)
versus occurrences that follow its usage as a contextual keyword (e.g.
the type `Box<dyn Trait>`).

In an ideal world the lint would parse the token stream in order to
distinguish such occurrences; but in general we cannot do this,
because a macro_rules definition does not specify what parsing
contexts the macro being defined is allowed to be used within.

So rather than put a lot of work into attempting to come up with a
more precise but still incomplete solution, I am just taking the short
cut of not linting any instance of `dyn` under a macro. This prevents
`rustfix` from injecting bugs into legal 2015 edition code.
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2019
…ed as keyword in test.

Back-story: After reflection this morning, I realized that the
previous form of this test would allow the macro invocation to treat
the `dyn` input as a raw-identifier rather than a keyword, and since
the input was discarded by that version of the macro, the test would
pass despite the detail that the input `dyn` should not have been
parsed as a raw-identifier.

This revision fixes that oversight, by actually *using* the macro
input to construct a `Box<dyn Trait>` type.
Review feedback asked for the test to be generalized to include macros
2.0; that generalization is dyn-2015-idents-in-decl-macros-unlinted.rs

As a drive-by, I also decided to revise the test to make it clear
*why* we cannot generally lint these cases. (I already had similar
demonstrations in dyn-2015-edition-keyword-ident-lint.rs, but it does
not hurt to try to emphasize matters.)

I also added some commentary on the cases where we could choose to
make the lint smarter, namely the situations where a macro is
*definitely* using `dyn` as an identifier (because it is using it as a
path component).
@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 30, 2019

📌 Commit 528366d has been approved by matthewjasper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 30, 2019
…rd-lint-under-macros, r=matthewjasper

skip dyn keyword lint under macros

This PR is following my own intuition that `rustfix` should never inject bugs into working code (even if that comes at the expense of it failing to fix things that will become bugs).

Fix rust-lang#56327
bors added a commit that referenced this pull request Mar 30, 2019
Rollup of 5 pull requests

Successful merges:

 - #59343 (rustc(codegen): uncache `def_symbol_name` prefix from `symbol_name`.)
 - #59380 (Fix invalid DWARF for enums when using ThinLTO)
 - #59463 (skip dyn keyword lint under macros)
 - #59539 (Fix infinite recursion)
 - #59544 (manifest: only include miri on the nightly channel)

Failed merges:

r? @ghost
@bors bors merged commit 528366d into rust-lang:master Mar 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

edition compatibility lints: Warning about dyn in a macro incorrectly
5 participants